feat: Added external link icon #4757

lavr001 · 2023-11-16T19:54:43Z

Description of changes

Added external link icon when isExternal prop is true

Before:

After:

Issue #4515 , if available

Description of how you validated changes

Checklist

Have read the Pull Request Guidelines
PR description included
Relevant documentation is changed or added (and PR referenced)
yarn test passes and tests are updated/added
No side effects or sideEffects field updated

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

changeset-bot · 2023-11-16T19:54:47Z

⚠️ No Changeset found

Latest commit: 5309237

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

packages/react/src/primitives/Link/Link.tsx

packages/ui/src/theme/css/component/link.scss

calebpollman · 2023-11-27T20:33:30Z

packages/react/src/primitives/Icon/icons/IconBoxArrowUpRight.tsx

+import { classNames } from '@aws-amplify/ui';
+import { ComponentClassName } from '@aws-amplify/ui';


This should only be a single import line

Unresolved, there are two import lines for the same package still. In the future, please refrain from resolving a conversation until the feedback has been addressed

Looks like there are two import lines for the same package across multiple primitives, I'll make sure to make it one line when done with other updates on this PR.

calebpollman · 2023-11-27T20:38:44Z

packages/react/src/primitives/Link/Link.tsx

@@ -25,6 +24,7 @@ const LinkPrimitive: Primitive<LinkProps, 'a'> = (
      {...rest}
    >
      {children}
+      {isExternal && <IconBoxArrowUpRight />}


Few things here:

Prefer ternary returning null if isExternal is falsy to prevent this expression from evaluating to false

How do i hide the icon?

How do i position the icon to the left of the children?

Updated to use a ternary operator

You can hide the icon when isExternal prop is set to false (default)

It's important to start with a word that indicates the action that results if customers click the link. Putting an icon at the beginning of the link disrupts the readability of the link quite massively, therefore it is a common convention to put icons after the actual link text

You can hide the icon when isExternal prop is set to false (default)

Sure, but what if the link is external and i don't want the icon?

It's important to start with a word that indicates the action that results if customers click the link. Putting an icon at the beginning of the link disrupts the readability of the link quite massively, therefore it is a common convention to put icons after the actual link text

What abt RTL languages?

This response is an opinion on the usage of the Link component but does not address the actual question

Good call on RTL languages. I'll work on adding a position prop, so devs can control the icon position

There is no need to create a position prop since the direction='rtl' can be set in the ThemeProvider and it will automatically applied to the Link component

@lavr001 The above question is still unanswered:

You can hide the icon when isExternal prop is set to false (default)

Sure, but what if the link is external and i don't want the icon?

@calebpollman Are you suggesting having a new prop called hasIcon that would be visible only when the link is external, so users can control if they want the icon or not?

@calebpollman Added new prop hideIcon, so now if the Link is external you have an option to show/hide the icon. Also added a new prop linkIconPosition, so you can position the icon left or right to the Link

…fy/amplify-ui into add-external-link-icon

reesscot · 2023-11-28T01:18:56Z

packages/ui/src/icons/box_arrow_up_right.svg

Where did this icon come from and why is it not centered like the other icons?

Since we don't typically use icons from the external libraries, I just looked online for a SVG path that forms the icon. I've added align-items: center to make the icon vertically aligned, but I can take another look to see what else could be done to make it more vertically centered

We are using a specific icon set from material, so we should be consistent and use the same one for this too. @dbanksdesign Do you remember which version we were using?

I checked SVG icons from material and they didn't really have a specific external link icon, so I grabbed another SVG

Where is this SVG taken from and what was the reasoning used in choosing it?

I simply googled svg path for external icon, since we don't usually pull in icons from an external source. Not sure what do you mean by reasoning. Are you suggesting some other approaches here?

calebpollman

Concerned overall with the approach taken here.

Rendering an icon by default for any Link components that have an external link could break customers layouts that have added an icon themselves, nor are we providing a means to override the icon or hide it. Blocking this until all feedback is addressed and the PR no longer contains breaking changes

calebpollman · 2023-12-15T23:25:49Z

packages/react/src/primitives/Icon/icons/IconBoxArrowUpRight.tsx

+import { classNames } from '@aws-amplify/ui';
+import { ComponentClassName } from '@aws-amplify/ui';


Unresolved, there are two import lines for the same package still. In the future, please refrain from resolving a conversation until the feedback has been addressed

calebpollman · 2023-12-15T23:27:04Z

packages/ui/src/icons/box_arrow_up_right.svg

Where is this SVG taken from and what was the reasoning used in choosing it?

calebpollman · 2023-12-15T23:32:46Z

packages/react/src/primitives/Link/Link.tsx

@@ -25,6 +24,7 @@ const LinkPrimitive: Primitive<LinkProps, 'a'> = (
      {...rest}
    >
      {children}
+      {isExternal && <IconBoxArrowUpRight />}


@lavr001 The above question is still unanswered:

You can hide the icon when isExternal prop is set to false (default)

Sure, but what if the link is external and i don't want the icon?

… add-external-link-icon

added external link icon

e2f5522

lavr001 requested a review from a team as a code owner November 16, 2023 19:54

dbanksdesign requested changes Nov 16, 2023

View reviewed changes

packages/react/src/primitives/Link/Link.tsx Outdated Show resolved Hide resolved

packages/ui/src/theme/css/component/link.scss Outdated Show resolved Hide resolved

packages/ui/src/theme/css/component/link.scss Outdated Show resolved Hide resolved

lavr001 added 2 commits November 21, 2023 16:02

removed external source for the link icon, small css updates

085c519

updating link test

c6d775b

lavr001 had a problem deploying to ci November 22, 2023 03:33 — with GitHub Actions Failure

lavr001 temporarily deployed to ci November 22, 2023 03:33 — with GitHub Actions Inactive

Merge branch 'main' into add-external-link-icon

0cbfb8e

lavr001 temporarily deployed to ci November 22, 2023 03:57 — with GitHub Actions Inactive

lavr001 requested review from dbanksdesign and calebpollman November 22, 2023 17:19

lavr001 changed the title ~~added external link icon~~ feat: Added external link icon Nov 27, 2023

calebpollman reviewed Nov 27, 2023

View reviewed changes

lavr001 added 2 commits November 27, 2023 14:04

small fixes based on comments

e9f156e

Merge branch 'add-external-link-icon' of https://github.com/aws-ampli…

318e9a6

…fy/amplify-ui into add-external-link-icon

lavr001 temporarily deployed to ci November 27, 2023 22:14 — with GitHub Actions Inactive

lavr001 requested review from calebpollman and reesscot November 27, 2023 22:15

reesscot reviewed Nov 28, 2023

View reviewed changes

lavr001 requested a review from reesscot December 5, 2023 00:06

calebpollman requested changes Dec 15, 2023

View reviewed changes

one line for the same package import

0aee71f

lavr001 requested a review from calebpollman January 4, 2024 18:08

lavr001 added 2 commits January 9, 2024 11:55

Merge branch 'main' of https://github.com/aws-amplify/amplify-ui into…

4e4ff7a

… add-external-link-icon

added hideIcon and linkIconPosition props

9cf0869

lavr001 had a problem deploying to ci January 9, 2024 23:04 — with GitHub Actions Failure

lavr001 temporarily deployed to ci January 9, 2024 23:04 — with GitHub Actions Inactive

Merge branch 'main' into add-external-link-icon

f57b263

lavr001 had a problem deploying to ci January 9, 2024 23:29 — with GitHub Actions Failure

lavr001 had a problem deploying to ci January 9, 2024 23:29 — with GitHub Actions Error

lavr001 temporarily deployed to ci January 9, 2024 23:29 — with GitHub Actions Inactive

Merge branch 'main' into add-external-link-icon

271658b

lavr001 had a problem deploying to ci January 10, 2024 18:00 — with GitHub Actions Failure

lavr001 had a problem deploying to ci January 10, 2024 18:00 — with GitHub Actions Error

lavr001 had a problem deploying to ci January 10, 2024 18:00 — with GitHub Actions Failure

Merge branch 'main' into add-external-link-icon

5309237

lavr001 had a problem deploying to ci January 11, 2024 18:50 — with GitHub Actions Failure

lavr001 had a problem deploying to ci January 11, 2024 18:50 — with GitHub Actions Error

lavr001 had a problem deploying to ci January 11, 2024 18:50 — with GitHub Actions Failure

reesscot closed this Jan 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Added external link icon #4757

feat: Added external link icon #4757

lavr001 commented Nov 16, 2023

changeset-bot bot commented Nov 16, 2023 •

edited

Loading

calebpollman Nov 27, 2023

calebpollman Dec 15, 2023

lavr001 Dec 20, 2023

calebpollman Nov 27, 2023

lavr001 Nov 27, 2023

calebpollman Nov 28, 2023

lavr001 Nov 28, 2023

lavr001 Dec 5, 2023

calebpollman Dec 15, 2023

lavr001 Dec 20, 2023

lavr001 Jan 9, 2024

reesscot Nov 28, 2023

lavr001 Nov 28, 2023

reesscot Dec 8, 2023

lavr001 Dec 14, 2023

calebpollman Dec 15, 2023

lavr001 Dec 21, 2023

calebpollman left a comment

calebpollman Dec 15, 2023

calebpollman Dec 15, 2023

calebpollman Dec 15, 2023

		import { classNames } from '@aws-amplify/ui';
		import { ComponentClassName } from '@aws-amplify/ui';

feat: Added external link icon #4757

feat: Added external link icon #4757

Conversation

lavr001 commented Nov 16, 2023

Description of changes

Issue #4515 , if available

Description of how you validated changes

Checklist

changeset-bot bot commented Nov 16, 2023 • edited Loading

⚠️ No Changeset found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebpollman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Nov 16, 2023 •

edited

Loading